-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[TS migration] Migrate 'ValidateLogin' page to TypeScript #33839
[TS migration] Migrate 'ValidateLogin' page to TypeScript #33839
Conversation
The lint step is failing - prettier diff |
@pasyukevich @VickyStash @kubabutkiewicz all feedback addressed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
session: OnyxEntry<SessionType>; | ||
}; | ||
|
||
type ValidateLoginPageProps = ValidateLoginPageOnyxProps & StackScreenProps<AuthScreensParamList, typeof SCREENS.VALIDATE_LOGIN>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ValidateLoginPage has platform specific implementations, create types.ts file with a shared type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! 👍
ValidateLoginPage.displayName = 'ValidateLoginPage'; | ||
|
||
export default withOnyx<ValidateLoginPageProps, ValidateLoginPageOnyxProps>({ | ||
session: {key: ONYXKEYS.SESSION}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Credentials were removed, please test the component carefully on native
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blazejkustra Credentials
was not being used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And honestly I don't know how to test this component on the native side because it seems this page is only used on web 🤷
@pac-guerreiro Typecheck is failing 🤔 |
@blazejkustra Resolved! 😄 |
Seems like no C+ is assigned here. I can review and checklist if needed. |
Thanks for offering @situchan |
account, | ||
credentials, | ||
route: { | ||
params: {accountID, validateCode}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please make sure that route.params
always exists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@situchan It's expected to be defined every time the component is mounted 😄
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2024-01-24.at.6.29.57.PM.movMacOS: Desktop |
@pasyukevich please merge main (for safety) when you get a chance. |
@pac-guerreiro you wrote QA Steps same as Tests but I am not sure QA team will follow that. They're not developers and don't know |
I agree with @situchan here @pac-guerreiro, it would be really nice to get the real steps to get this pages to show up, do you think we can do that? |
Sorry, I forgot to write clear instructions for QA 😞 Sadly, QA team won't be able to test all the modals because it's not easy to replicate the states without directly manipulating the data (which it's not possible in staging and production). I'll just write which page in staging you'll need to access in order to test this work. You can also check the visual representation of the modals by taking a look at the screenshots section of this PR 😄 |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/rlinoz in version: 1.4.33-0 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.4.33-5 🚀
|
Details
Fixed Issues
$#25189
PROPOSAL: N/A
Tests
web
, navigate tohttps://dev.new.expensify.com:8082/v/1/222222
ValidateCodeModal
. Otherwise, runwindow.Onyx.merge("session", {autoAuthState: "not-started"});
on the browser console. Confirm if everything is rendered correctly.ExpiredValidateCodeModal
, runwindow.Onyx.merge("session", {autoAuthState: "failed"});
on the browser console. Confirm if everything is rendered correctly.JustSignedInModal
, runwindow.Onyx.merge("session", {autoAuthState: "just-signed-in"});
on the browser console. Confirm if everything is rendered correctly.Offline tests
N/A
QA Steps
web
, navigate tohttps://staging.new.expensify.com/v/1/222222
ValidateCodeModal
.PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
MacOS: Chrome / Safari
ValidateCodeModal
ExpiredValidateCodeModal
JustSignedInModal